Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MNG-7433] Warn if in parallel build aggregator Mojo found #730

Closed
wants to merge 3 commits into from
Closed

[MNG-7433] Warn if in parallel build aggregator Mojo found #730

wants to merge 3 commits into from

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Apr 27, 2022

No description provided.

@gnodet gnodet requested a review from cstamas April 27, 2022 09:47
@cstamas
Copy link
Member

cstamas commented Apr 27, 2022

Just FYI: this would need to go in all branches, not just master, where the lock is present, so 3.8.x, 3.9.x and master.

michael-o
michael-o requested changes May 6, 2022
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the message to look similar to this:

maven/maven-core/src/main/java/org/apache/maven/lifecycle/internal/builder/BuilderCommon.java

Lines 144 to 152 in faf5d5d

logger.warn( "*****************************************************************" );
logger.warn( "* Your build is requesting parallel execution, but project *" );
logger.warn( "* contains the following plugin(s) that have goals not marked *" );
logger.warn( "* as @threadSafe to support parallel building. *" );
logger.warn( "* While this /may/ work fine, please look for plugin updates *" );
logger.warn( "* and/or request plugins be made thread-safe. *" );
logger.warn( "* If reporting an issue, report it against the plugin in *" );
logger.warn( "* question, not against maven-core *" );
logger.warn( "*****************************************************************" );

@gnodet gnodet requested a review from michael-o May 11, 2022 12:16
michael-o
michael-o requested changes May 11, 2022
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a separate ticket for this.

Question: I guess this must also go into 3.9.0 and master?

@gnodet
Copy link
Contributor Author

gnodet commented May 12, 2022

Please create a separate ticket for this.

For what exactly ? The terminal width thing ? You'd prefer the terminal width adjustment to be only in 3.9.x / master ?

Question: I guess this must also go into 3.9.0 and master?

Yes, the warning should be in all 3 supported branches.

Fwiw, I've committed the improvement with the terminal width. The thread safety warning now looks like the following in my terminal:

[INFO] -----------------------< org.apache.maven:maven >-----------------------
[INFO] Building Apache Maven 3.8.6-SNAPSHOT                              [1/15]
[INFO] --------------------------------[ pom ]---------------------------------
[WARNING] *******************************************************************************************************************************************************************
[WARNING] * Your build is requesting parallel execution, but project contains the following plugin(s) that have goals not marked as @threadSafe to support parallel         *
[WARNING] * building.                                                                                                                                                       *
[WARNING] * While this /may/ work fine, please look for plugin updates and/or request plugins be made thread-safe.                                                          *
[WARNING] * If reporting an issue, report it against the plugin in question, not against maven-core.                                                                        *
[WARNING] *******************************************************************************************************************************************************************
[WARNING] The following plugins are not marked @threadSafe in Apache Maven:
[WARNING]   - org.apache.rat:apache-rat-plugin:0.13
[WARNING] Enable debug to see more precisely which goals are not marked @threadSafe.
[WARNING] *******************************************************************************************************************************************************************
[INFO]

Btw, having maven displaying such warnings during its own build... !!

@gnodet
Copy link
Contributor Author

gnodet commented May 12, 2022

I wonder if it wouldn't be better to fix enhance the ExecutionEventLogger at the same time, the one that displays:

[INFO] -----------------------< org.apache.maven:maven >-----------------------
[INFO] Building Apache Maven 3.8.6-SNAPSHOT                              [1/15]
[INFO] --------------------------------[ pom ]---------------------------------

@michael-o
Copy link
Member

Please create a separate ticket for this.

For what exactly ? The terminal width thing ? You'd prefer the terminal width adjustment to be only in 3.9.x / master ?

For the logging message and its intent in 3.8.x, 3.9.x and master. The goal is to show the user in the changelog that such a warning in this specific case is being implemented. Then MNG-7433 will partially be addressed through this log message and user requirement to restructure code/layout of project.

@gnodet
Copy link
Contributor Author

gnodet commented May 12, 2022

I've raised #736 with a new JIRA issue for the warning.
The terminal width layout does not really make sense unless the default logging also adapts, I'll raise a JIRA for that and will provide a PR for 3.9.x / master.

@gnodet gnodet closed this May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o requested changes

@cstamas cstamas cstamas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gnodet @cstamas @michael-o